test(integ-test): stabilize deterministic fixtures across shards by mengweieric · Pull Request #5730 · opensearch-project/sql

mengweieric · 2026-08-29T04:13:17Z

Summary

A set of integration tests selected rows through head, limit, shard-local representative choice, or unordered collection output, then asserted exact single-shard results.

This change selects intended fixture rows with unique keys, compares unordered collections as multisets, and uses numeric tolerances only for distributed floating-point or geo-point encoding differences. Schema, cardinality, grouping, source-value, and command-specific assertions remain exact.

No production behavior is modified.

Validation

Verified on an external cluster forced to five primary shards
Exercised direct, paginating, and independently checked no-pushdown paths as applicable
All modified tests passed targeted five-shard validation
spotlessCheck, compileTestJava, and git diff --check pass

github-actions · 2026-08-29T04:14:40Z

PR Reviewer Guide 🔍

(Review updated until commit `91bfdd2`)

Here are some key observations to aid the review process:

🧪 No relevant tests
🔒 No security concerns identified
✅ No TODO sections
🔀 No multiple PR themes
⚡ Recommended focus areas for review Possible Issue The test `testJoinWithFieldListMaxEqualsOne` now asserts only 4 deterministic rows and checks total count is 5, but does not verify David's country. If David's country is genuinely undefined due to max=1 selecting an arbitrary match, the test should explicitly document that both USA and Canada are valid outcomes. Without this, a future reader may incorrectly assume the test is incomplete or that David's country should be deterministic. The same issue appears in `testJoinWhenLegacyNotPreferred`. schema("month", "int"), schema("occupation", "string"), schema("salary", "int")); JSONObject actual2 = executeQuery( String.format( "source=%s \| join type=inner max=1 name,year,month %s \| fields name, country", TEST_INDEX_STATE_COUNTRY, TEST_INDEX_OCCUPATION)); // max=1 keeps a single OCCUPATION match per left row, and the co-named country column resolves // to the OCCUPATION (right) side (see Jake -> England). David has two OCCUPATION rows that are // identical on the join field list (name=David, year=2023, month=4) and differ only in the // projected country (USA from the Doctor row, Canada from the Unemployed row), so which one // survives max=1 is undefined and cannot be disambiguated by any ordering. Assert the four // deterministic rows and that max=1 collapses David's two matches to a single row (total of 5, // not 6); do not pin David's undefined country. verifyNumOfRows(actual2, 5); verifyDataRowsSome( actual2, rows("Jake", "England"), rows("Jane", "Canada"), rows("John", "Canada"), rows("Hello", "USA")); } Possible Issue The new `rowsUnordered` matcher recursively compares JSON values treating all arrays as unordered multisets. If a test legitimately requires a specific array order (e.g., a sorted result or a sequence where order matters), this matcher will incorrectly pass even when the order is wrong. The matcher is scoped to this test class, but if copied elsewhere or if a future test in this class needs ordered array comparison, it will silently accept incorrect results. private static TypeSafeMatcher<JSONArray> rowsUnordered(Object... expectedObjects) { return new TypeSafeMatcher<>() { @Override protected boolean matchesSafely(JSONArray array) { JSONArray expected = new JSONArray(expectedObjects); if (array.length() != expected.length()) { return false; } // Compare top-level cells positionally so column identity is preserved; only descend into // nested arrays/objects with order-insensitive comparison. for (int i = 0; i < expected.length(); i++) { if (!jsonEqualsIgnoringOrder(array.get(i), expected.get(i))) { return false; } } return true; } @Override public void describeTo(Description description) { description.appendText(new JSONArray(expectedObjects).toString()); } }; } Possible Issue The `verifyDataRows` method now compares rows as multisets by sorting their string representations. If two rows have identical string serializations but differ in actual structure (e.g., nested JSON objects with different key orders that serialize identically), this comparison will falsely report them as equal. This could mask genuine differences in row content. public void verifyDataRows(JSONArray dataRowsOne, JSONArray dataRowsTwo) { // The paginated and non-paginated APIs must return the same complete set of rows, but none of // these queries impose a total ordering: three have no ORDER BY at all, and the ORDER BY // variants sort on a non-unique column (balance), so the order among equal-key rows is not // contractual. On a multi-shard index the scroll (paginated) and search (non-paginated) paths // materialize the same rows in different orders, so JSONArray.similar()'s positional comparison // fails even though the row sets are equivalent. Compare as multisets to assert complete-row // equivalence (full coverage and totals) without depending on an order that is not contractual. List<String> rowsOne = new ArrayList<>(); dataRowsOne.iterator().forEachRemaining(o -> rowsOne.add(o.toString())); List<String> rowsTwo = new ArrayList<>(); dataRowsTwo.iterator().forEachRemaining(o -> rowsTwo.add(o.toString())); rowsOne.sort(null); rowsTwo.sort(null); assertEquals(rowsOne, rowsTwo); }

github-actions · 2026-08-31T19:22:22Z

Persistent review updated to latest commit 60e3690

github-actions · 2026-08-31T19:22:56Z

PR Code Suggestions ✨

Latest suggestions up to 91bfdd2

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Use tolerance for floating-point comparison Comparing floating-point numbers with `==` can produce false negatives due to precision errors. Use a tolerance-based comparison (e.g., `Math.abs(a - b) < EPSILON`) to handle rounding differences that arise from different accumulation orders across shards. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [171-181] +private static final double EPSILON = 1e-9; + private static boolean scalarEquals(Object a, Object b) { boolean aNull = a == null \|\| a == JSONObject.NULL; boolean bNull = b == null \|\| b == JSONObject.NULL; if (aNull \|\| bNull) { return aNull && bNull; } if (a instanceof Number && b instanceof Number) { - return ((Number) a).doubleValue() == ((Number) b).doubleValue(); + double aVal = ((Number) a).doubleValue(); + double bVal = ((Number) b).doubleValue(); + return Math.abs(aVal - bVal) < EPSILON; } return a.toString().equals(b.toString()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential issue with floating-point comparison using `==`. However, the `scalarEquals` method is used within a custom matcher for comparing JSON values in tests, where exact equality is often acceptable for most numeric values. The tolerance-based approach would be more robust for floating-point comparisons, but the current implementation may be intentional for test assertions. The score reflects that this is a valid improvement for numerical stability, though not critical given the test context.	Medium

Previous suggestions

Suggestions up to commit 6ba546d

Category	Suggestion	Impact
General	Use tolerance-based floating-point comparison Floating-point equality comparison using `==` is unreliable due to precision issues. Replace the direct equality check with a tolerance-based comparison (e.g., `Math.abs(diff) < EPSILON`) to avoid false negatives when comparing numeric values that should be considered equal. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [171-181] private static boolean scalarEquals(Object a, Object b) { boolean aNull = a == null \|\| a == JSONObject.NULL; boolean bNull = b == null \|\| b == JSONObject.NULL; if (aNull \|\| bNull) { return aNull && bNull; } if (a instanceof Number && b instanceof Number) { - return ((Number) a).doubleValue() == ((Number) b).doubleValue(); + double diff = ((Number) a).doubleValue() - ((Number) b).doubleValue(); + return Math.abs(diff) < 1E-9; } return a.toString().equals(b.toString()); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies a potential issue with floating-point equality comparison using `==`. However, the context shows this is a test utility for comparing JSON values where exact equality may be acceptable for most test cases. The suggested tolerance-based comparison would improve robustness, but the impact is moderate since this is test code and the current implementation may be sufficient for the test scenarios.	Medium

Suggestions up to commit 31f8a2a

Category	Suggestion	Impact
General	Use tolerance for floating-point comparison Comparing floating-point numbers with `==` can produce incorrect results due to precision issues. Use a tolerance-based comparison (e.g., `Math.abs(diff) < EPSILON`) to handle rounding errors properly. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [171-181] private static boolean scalarEquals(Object a, Object b) { boolean aNull = a == null \|\| a == JSONObject.NULL; boolean bNull = b == null \|\| b == JSONObject.NULL; if (aNull \|\| bNull) { return aNull && bNull; } if (a instanceof Number && b instanceof Number) { - return ((Number) a).doubleValue() == ((Number) b).doubleValue(); + double aVal = ((Number) a).doubleValue(); + double bVal = ((Number) b).doubleValue(); + return Math.abs(aVal - bVal) < 1e-9; } return a.toString().equals(b.toString()); } Suggestion importance[1-10]: 7 __ Why: Valid concern about floating-point comparison precision. The suggestion to use tolerance-based comparison (`Math.abs(aVal - bVal) < 1e-9`) is appropriate for test assertions where rounding errors can occur. This improves robustness without changing the test's intent.	Medium
	Use structural JSON comparison Converting JSON objects to strings for comparison can produce false positives when different JSON structures serialize to identical strings. Use a structural comparison that respects JSON semantics instead of string equality. integ-test/src/test/java/org/opensearch/sql/legacy/CursorIT.java [501-516] public void verifyDataRows(JSONArray dataRowsOne, JSONArray dataRowsTwo) { ... - List<String> rowsOne = new ArrayList<>(); - dataRowsOne.iterator().forEachRemaining(o -> rowsOne.add(o.toString())); - List<String> rowsTwo = new ArrayList<>(); - dataRowsTwo.iterator().forEachRemaining(o -> rowsTwo.add(o.toString())); - rowsOne.sort(null); - rowsTwo.sort(null); - assertEquals(rowsOne, rowsTwo); + List<JSONArray> rowsOne = new ArrayList<>(); + dataRowsOne.iterator().forEachRemaining(o -> rowsOne.add((JSONArray) o)); + List<JSONArray> rowsTwo = new ArrayList<>(); + dataRowsTwo.iterator().forEachRemaining(o -> rowsTwo.add((JSONArray) o)); + rowsOne.sort(Comparator.comparing(JSONArray::toString)); + rowsTwo.sort(Comparator.comparing(JSONArray::toString)); + assertEquals(rowsOne.size(), rowsTwo.size()); + for (int i = 0; i < rowsOne.size(); i++) { + assertTrue(rowsOne.get(i).similar(rowsTwo.get(i))); + } } Suggestion importance[1-10]: 5 __ Why: The suggestion correctly identifies that string comparison of JSON can be fragile. Using `JSONArray.similar()` for structural comparison is more robust. However, the implementation still sorts by `toString()` which could be problematic. A better approach would use a custom comparator that handles JSON structure, but the suggested improvement is still an upgrade over pure string equality.	Low
	Optimize array comparison algorithm The nested loop with iterator removal has O(n²) time complexity for array comparison. For large arrays with many elements, this could cause performance issues. Consider using a frequency map approach to achieve O(n) complexity while still handling duplicates correctly. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [146-169] private static boolean jsonArrayEqualsIgnoringNestedOrder(JSONArray actual, JSONArray expected) { if (actual.length() != expected.length()) { return false; } - List<Object> remaining = new ArrayList<>(); + Map<String, Integer> expectedCounts = new HashMap<>(); for (int i = 0; i < expected.length(); i++) { - remaining.add(expected.get(i)); + String key = expected.get(i).toString(); + expectedCounts.merge(key, 1, Integer::sum); } for (int i = 0; i < actual.length(); i++) { - Object actualElement = actual.get(i); - boolean matched = false; - for (Iterator<Object> it = remaining.iterator(); it.hasNext(); ) { - if (jsonEqualsIgnoringOrder(actualElement, it.next())) { - it.remove(); - matched = true; - break; - } - } - if (!matched) { + String key = actual.get(i).toString(); + Integer count = expectedCounts.get(key); + if (count == null \|\| count == 0) { return false; } + expectedCounts.put(key, count - 1); } return true; } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies a performance concern with the O(n²) nested loop. However, the proposed frequency map approach using `toString()` as keys would break the deep structural comparison that `jsonEqualsIgnoringOrder` provides. The current implementation correctly handles nested arrays/objects recursively, while the suggested approach would only compare serialized strings, losing semantic correctness.	Low

Suggestions up to commit b9c4c8a

Category	Suggestion	Impact
Possible issue	Handle type mismatches in recursive comparison The recursive comparison does not handle the case where `a` and `b` are of different types (e.g., one is a `JSONArray` and the other is a `JSONObject`). This will fall through to `scalarEquals`, which may produce incorrect results. Add an explicit type mismatch check before the recursive calls. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [116-134] private static boolean jsonEqualsIgnoringOrder(Object a, Object b) { if (a instanceof JSONArray && b instanceof JSONArray) { return jsonArrayEqualsIgnoringNestedOrder((JSONArray) a, (JSONArray) b); + } + if (a instanceof JSONArray \|\| b instanceof JSONArray) { + return false; } if (a instanceof JSONObject && b instanceof JSONObject) { JSONObject ao = (JSONObject) a; JSONObject bo = (JSONObject) b; if (ao.keySet().size() != bo.keySet().size()) { return false; } for (String key : ao.keySet()) { if (!bo.has(key) \|\| !jsonEqualsIgnoringOrder(ao.get(key), bo.get(key))) { return false; } } return true; } + if (a instanceof JSONObject \|\| b instanceof JSONObject) { + return false; + } return scalarEquals(a, b); } Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the method doesn't explicitly handle type mismatches between `JSONArray` and `JSONObject` before falling through to `scalarEquals`. Adding explicit checks prevents incorrect comparisons when `a` and `b` are of different JSON container types, improving correctness and clarity.	Medium

Suggestions up to commit 60e3690

Category	Suggestion	Impact
General	Handle type mismatch in comparison The method does not handle the case where `a` and `b` are of different types (e.g., one is a `JSONArray` and the other is a `JSONObject`). This will cause the method to fall through to `scalarEquals`, which may produce incorrect results. Add an explicit type mismatch check before the scalar comparison. integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java [116-134] private static boolean jsonEqualsIgnoringOrder(Object a, Object b) { if (a instanceof JSONArray && b instanceof JSONArray) { return jsonArrayEqualsIgnoringNestedOrder((JSONArray) a, (JSONArray) b); } if (a instanceof JSONObject && b instanceof JSONObject) { JSONObject ao = (JSONObject) a; JSONObject bo = (JSONObject) b; if (ao.keySet().size() != bo.keySet().size()) { return false; } for (String key : ao.keySet()) { if (!bo.has(key) \|\| !jsonEqualsIgnoringOrder(ao.get(key), bo.get(key))) { return false; } } return true; } + if ((a instanceof JSONArray \|\| a instanceof JSONObject) != (b instanceof JSONArray \|\| b instanceof JSONObject)) { + return false; + } return scalarEquals(a, b); } Suggestion importance[1-10]: 3 __ Why: The suggestion correctly identifies that type mismatches between `JSONArray` and `JSONObject` are not explicitly handled before falling through to `scalarEquals`. However, the current implementation already handles this implicitly: if `a` and `b` are different types (one is `JSONArray`, the other is `JSONObject`), neither of the first two conditions will match, and `scalarEquals` will return `false` via `toString().equals()`. The explicit check adds clarity but doesn't fix a bug.	Low

github-actions · 2026-09-01T17:06:28Z

Persistent review updated to latest commit b9c4c8a

github-actions · 2026-09-02T04:10:57Z

Persistent review updated to latest commit 31f8a2a

github-actions · 2026-09-04T20:14:37Z

Persistent review updated to latest commit 6ba546d

Select fixture rows by unique keys, compare unordered collections as multisets, and use numeric tolerances where distributed accumulation or encoding is lossy. Keep exact schema, cardinality, and source-value assertions. Signed-off-by: Eric Wei <menwe@amazon.com>

Signed-off-by: Eric Wei <menwe@amazon.com>

Q10 selected its single top row with sort on an aggregated double alone, so a tie would let head 1 pick either row. Sort on c_custkey as well to pin the selected row on any shard layout. Signed-off-by: Eric Wei <menwe@amazon.com>

A JSONArray or JSONObject on only one side fell through to a toString comparison, so a container whose serialized form equalled a scalar string matched incorrectly. Reject that case and cover it with tests. Signed-off-by: Eric Wei <menwe@amazon.com>

The two coalesce tests now sort by the unique age field before applying head, so they no longer depend on unstable encounter order. Remove the HEAD_WITHOUT_STABLE_SORT capability gate and run them on the analytics-engine route again. Signed-off-by: Eric Wei <menwe@amazon.com> Signed-off-by: menwe <menwe@amazon.com>

github-actions · 2026-09-04T22:27:16Z

Persistent review updated to latest commit 91bfdd2

mengweieric requested review from LantaoJin, RyanL1997, Swiddis, acarbonetto, ahkcs, anirudha, dai-chen, joshuali925, noCharger, penghuo, ps48, qianheng-aws, songkant-aws, vamsimanohar, ykmr1224 and yuancu as code owners August 29, 2026 04:13

mengweieric added the testing Related to improving software testing label Aug 29, 2026

dai-chen reviewed Sep 1, 2026

View reviewed changes

Comment thread integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteArrayFunctionIT.java

mengweieric mentioned this pull request Sep 2, 2026

[TEST] Add a multi-shard integration test CI lane #5738

Open

dai-chen reviewed Sep 3, 2026

View reviewed changes

Comment thread integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLGraphLookupIT.java

RyanL1997 reviewed Sep 4, 2026

View reviewed changes

Comment thread integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalcitePPLEnhancedCoalesceIT.java

Comment thread integ-test/src/test/java/org/opensearch/sql/calcite/remote/CalciteSortCommandIT.java

mengweieric added 3 commits September 4, 2026 22:24

test(integ-test): preserve graphLookup column positions

599421c

Signed-off-by: Eric Wei <menwe@amazon.com>

test(integ-test): clarify graphLookup matcher scope

ec7805b

Signed-off-by: Eric Wei <menwe@amazon.com>

mengweieric added 4 commits September 4, 2026 22:24

test(integ-test): stabilize remaining deterministic queries

8c855c7

Signed-off-by: Eric Wei <menwe@amazon.com>

mengweieric force-pushed the menwe/multi-shard-deterministic-fixtures branch from 6ba546d to 91bfdd2 Compare September 4, 2026 22:25

Navigation Menu

Uh oh!

test(integ-test): stabilize deterministic fixtures across shards - #5730

test(integ-test): stabilize deterministic fixtures across shards#5730
mengweieric wants to merge 7 commits into
opensearch-project:mainfrom
mengweieric:menwe/multi-shard-deterministic-fixtures

mengweieric commented Aug 29, 2026

Uh oh!

github-actions Bot commented Aug 29, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 31, 2026

Uh oh!

github-actions Bot commented Aug 31, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 1, 2026

Uh oh!

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

mengweieric commented Aug 29, 2026

Summary

Validation

Uh oh!

github-actions Bot commented Aug 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Reviewer Guide 🔍

(Review updated until commit 91bfdd2)

Uh oh!

github-actions Bot commented Aug 31, 2026

Uh oh!

github-actions Bot commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

github-actions Bot commented Sep 1, 2026

Uh oh!

Uh oh!

github-actions Bot commented Sep 2, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

github-actions Bot commented Sep 4, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Aug 29, 2026 •

edited

Loading

(Review updated until commit `91bfdd2`)

github-actions Bot commented Aug 31, 2026 •

edited

Loading